Skip to content
This repository was archived by the owner on Feb 5, 2025. It is now read-only.

WordPressComSiteInfo: update setting hasJetpack from remote response #110

Merged
merged 1 commit into from
Jul 24, 2019
Merged

WordPressComSiteInfo: update setting hasJetpack from remote response #110

merged 1 commit into from
Jul 24, 2019

Conversation

@jaclync
Copy link
Contributor

@jaclync jaclync commented Jul 18, 2019 •
edited
Loading

Fixes woocommerce/woocommerce-ios#1113

Background

In this PR, we switched the API call to fetch site info during site address sign in flow from fetchSiteInfo to fetchUnauthenticatedSiteInfoForAddress in order to accommodate both public and private sites (fetchSiteInfo only works for public sites). However, the responses from these two calls are very different.

With fetchSiteInfo (what we called before for WP.com sites), the response is like:

{
    "ID": 161199960,
    "name": "fun testing",
    "description": "",
    "URL": "https://funtestingusa.wpcomstaging.com",
    "jetpack": true,
    ...
}

And with fetchUnauthenticatedSiteInfoForAddress, the response looks like:

{
    "urlAfterRedirects": "https://funtestingusa.wpcomstaging.com",
    "isWordPress": true,
    "hasJetpack": true,
    "isWordPressDotCom": true
    ...
}

Originally, the site info’s hasJetpack is decoded from the response’s jetpack field, but now this info lives in the response's hasJetpack field. Thus, site info’s hasJetpack field is always false.

This is not a problem in WPiOS because WPiOS does not check any other fields in the site info before proceeding to the next screen vs. WCiOS only continues when site info's hasJetpack field is true in the authenticator delegate method. That’s why the site address is not considered valid and stuck on the site address screen in WCiOS.

Changes

  • Updated how we populate hasJetpack field in WordPressComSiteInfo from remote response's jetpack field to hasJetpack field
  • Notes: there are more fields that are not populated correctly with the new API response (e.g. name, url, icon), but I'd like to keep this PR small to just fix the sign in issue in WCiOS

Testing

In WCiOS branch

  • Sign in with site address --> should be able to complete the flow to enter a WC store

In WPiOS branch

  • Sign in with site address --> should work as before

@jaclync jaclync self-assigned this Jul 18, 2019
@jaclync jaclync mentioned this pull request Jul 18, 2019
Closed
@jaclync
Copy link
Contributor Author

jaclync commented Jul 18, 2019 •
edited
Loading

I just tested on WPiOS, but signing in with site address didn't work for me on the latest develop, the testing branch, and also on the latest app store build. The error occurs on a different screen, when entering username/password after site address. Do you see the same issue, or maybe I missed something?

Update: it was due to a *.wpcomstaging.com address. It worked when I signed in with a *.wordpress.com address for my site.

Simulator Screen Shot - iPhone SE - 2019-07-18 at 18 22 36

@ctarda
Copy link
Contributor

ctarda commented Jul 19, 2019

@jaclync I just tested with WooCommerce and for this store:

I got this error back:

To be fair, that is the same result I get when trying to log in with our develop

Also, the colour palette has changed completely, this is what we have on develop right now:

@jaclync
Copy link
Contributor Author

jaclync commented Jul 19, 2019

@ctarda thanks for testing!

I just tested with WooCommerce and for this store. To be fair, that is the same result I get when trying to log in with our develop

Hmm are you able to sign in with the same address in production from the App Store? Also, could you try again including http:// in the address?

Also, the colour palette has changed completely, this is what we have on develop right now

Yep if we update the authenticator pod to the latest with color changes (still in beta), then we'd update the colors for the new color variables (buttonTextColor, buttonTextHighlightColor, placeholderColor etc.). I set these colors to some native colors just for testing, since I didn't know the right colors right away 😛 If we want to update the authenticator pod to a newer stable version like 1.5.3 or 1.6.0, we don't have to make the color changes.

@yaelirub
Copy link
Contributor

yaelirub commented Jul 19, 2019

@jaclync , I'm going to review and test soon but just a small comment on the description of this issue:

we switched the API call to fetch site info during site address sign in flow from fetchUnauthenticatedSiteInfoForAddress to fetchSiteInfo

We actually switched from fetchSiteInfo to fetchUnauthenticatedSiteInfoForAddress

@jaclync jaclync mentioned this pull request Jul 19, 2019
Closed
@jaclync
Copy link
Contributor Author

jaclync commented Jul 19, 2019

We actually switched from fetchSiteInfo to fetchUnauthenticatedSiteInfoForAddress

@yaelirub my bad! Just updated the PR description :)

yaelirub
yaelirub approved these changes Jul 19, 2019
View reviewed changes
Copy link
Contributor

@yaelirub yaelirub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested on WPiOS by checking out the branch issue/wcios-1113-authenticator-sign-in-site-address-testing
Login worked with user and password and with site.

@ctarda
Copy link
Contributor

ctarda commented Jul 23, 2019

@jaclync sorry for the delay. I still can not login to store.ctarda.com, neither with nor without the scheme, but I can not do it in production either (same error)

I tested with a different site, without a subdomain and it worked.

ctarda
ctarda approved these changes Jul 23, 2019
View reviewed changes
Copy link
Contributor

@ctarda ctarda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@jaclync
Copy link
Contributor Author

jaclync commented Jul 24, 2019

thanks for reviewing & testing! 😃 merging now and closing the testing PRs right after

@jaclync jaclync merged commit 4529937 into develop Jul 24, 2019
@jaclync jaclync deleted the issue/wcios-1113-sign-in-site-address branch July 24, 2019 08:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@mindgraffiti mindgraffiti Awaiting requested review from mindgraffiti

2 more reviewers

@yaelirub yaelirub yaelirub approved these changes

@ctarda ctarda ctarda approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jaclync jaclync

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

WordPressAuthenticator: fix issue with logging in by site address starting version 1.5.3

4 participants

@jaclync @ctarda @yaelirub